Repository navigation
feat: Add RAM Frequency and Type to Environment Summary (#3041) - #3281
cloudsealed wants to merge 16 commits into
Conversation
…ll raw SMBIOS table
timcassell
left a comment
There was a problem hiding this comment.
Can you implement it for MacOS also?
| return null; | ||
| } | ||
|
|
||
| private static (long? FrequencyMHz, string? MemoryType) GetLinuxOrWindowsMemoryDetails() |
There was a problem hiding this comment.
It looks like this should be 2 separate methods.
There was a problem hiding this comment.
You did not address. GetLinuxOrWindowsMemoryDetails should be split to GetLinuxMemoryDetails and GetWindowsMemoryDetails.
Implement GetMacMemoryDetails() using system_profiler SPMemoryDataType to extract memory speed and type information on macOS, matching Windows and Linux implementations. Addresses feedback from @timcassell on PR dotnet#3281. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Hi @timcassell, I implemented macOS support as requested. The new The implementation:
Ready for review. Let me know if you'd like any adjustments. |
…cs() Extract frequency and memory type detection into a dedicated method to match the architectural pattern used by Windows (GetWindowsSmbiosMemoryDetails) and Linux (GetLinuxOrWindowsMemoryDetails). Addresses feedback from @timcassell that implementation should use separate methods. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Updated to address your feedback about separate methods. Refactored the macOS implementation: ✅ Created dedicated Ready for review. Commit: |
Use ProcessHelper.RunAndReadOutput instead of manual ProcessStartInfo calls in GetMacMemory() and GetMacMemorySpecs() to avoid potential deadlock when system_profiler output exceeds the stdout buffer size. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
I've confirmed latest CI logs. Though, it seems failed to gets detailed memory information and fallback path is used. And
|
- Convert MHz to MT/s (Megatransfers per second) for DDR memory types - Update FrequencyMHz property to FrequencyMts - Apply 2x multiplier: MT/s = MHz × 2 for DDR memory - Remove "Unspecified" filter from Linux memory type detection - Update unit label in output from MHz to MT/s Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Use non-whitespace pattern (\S+) instead of uppercase-only pattern to properly capture memory type strings like UDIMM, LRDIMM, etc. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
It wasn't explicitly requested in the issue, but it would be nice to include memory timings also, if the info is available. |
- Add CasLatency property to PhysicalMemoryInfo - Extract CAS latency from SMBIOS Type 17 offset 0x1C on Windows - Display in format: 8GB (DDR4-6400MT/s-CAS16) - Update unit tests to verify CAS latency property Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
| freq = speedMhz * 2; // Convert MHz to MT/s | ||
| } | ||
|
|
||
| var typeMatch = Regex.Match(output, @"Type:\s+(\S+)"); |
There was a problem hiding this comment.
On macos(arm64) CI environment.
It looks like not able to gets memory details information.
Memory:
Memory: 7 GB
Type: Unknown
Manufacturer: Unknown
And following information is printed.
Memory: 7 GB (Unknown), 0.56 GB Available
I though it's better to remove (Unknown) because it's confusing what's Unknow means.
There was a problem hiding this comment.
On macos(x64).
It looks like memory type information can't get as expected.
And printed as following.
Memory: 14 GB (RAM-3200MT/s), 0.6 GB Available
| { | ||
| try | ||
| { | ||
| string? output = ProcessHelper.RunAndReadOutput("system_profiler", "SPMemoryDataType"); |
There was a problem hiding this comment.
I've not have macos environment thought.
It looks like system_profiler SPMemoryDataType command results can be localized.
So it might be better to use -json option. and parse results with System.Text.Json
Replace regex-based parsing of localized system_profiler text with JSON output. This eliminates issues with different locales and handles varying output formats on different macOS versions. Changes: - Use system_profiler -json SPMemoryDataType instead of text parsing - Parse JSON with System.Text.Json for robustness - Filter out "Unknown" memory types from output - Extract speed and type from either direct properties or _properties array - Gracefully handle missing or malformed output Addresses feedback on regex fragility and locale-specific output variations.
| byte cas = buffer[offset + 0x1C]; | ||
| if (cas > 0 && cas < 100) // Reasonable CAS latency range | ||
| { | ||
| casLatency = cas; |
There was a problem hiding this comment.
At least in CI, this value is not included (I haven't tested locally). Unsure if the cas < 100 guard is really necessary. And cas > 0 is redundant (byte is [0, 255]).
Display 'GB Total' instead of just 'GB' to clarify total memory vs available memory. Output now shows: '16 GB Total (DDR4-6400MT/s-CAS16), 8 GB Available'
There was a problem hiding this comment.
SMBIOS specification define this speed unit as MT/s
So this logic for DDR memory should be removed.
SMBIOS specification defines memory speed at offset 0x15 as MT/s, not MHz. Remove the DDR multiplication logic (speedMhz * 2) since the value is already in the correct unit. Also simplify CAS latency validation by removing the unnecessary cas < 100 guard, keeping only cas > 0. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| // CAS Latency at offset 0x1C (BYTE) |
There was a problem hiding this comment.
As far as I've confirmed SMBIOS Specification.
https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_3.10.0.pdf
0x1C is defined as Extended Size field (That is used for represent size over 32GB)
And it looks like there is no field to get CAS Latency
Fixes #3041
Description
This PR implements the community-requested feature to read and display RAM frequency (MHz) and memory type (e.g., DDR4) in the environment summary.
It updates
PhysicalMemoryInfoto parse WMI (Win32_PhysicalMemory) on Windows and/usr/sbin/dmidecodeon Linux.Testing
PhysicalMemoryInfoTeststo validate memory info parsing without allocations.dotnet testpassed 100% on net10.0 and net8.0.